Skip to content

rustc_data_structures cleanups#124831

Merged
bors merged 7 commits intorust-lang:masterfrom
nnethercote:rustc_data_structures-cleanups
May 9, 2024
Merged

rustc_data_structures cleanups#124831
bors merged 7 commits intorust-lang:masterfrom
nnethercote:rustc_data_structures-cleanups

Conversation

@nnethercote
Copy link
Contributor

Some improvements I found while looking through this code.

r? @michaelwoerister

Normal `use` items are nicer.
- `use` before `mod`
- `pub` before `non-pub`
- Alphabetical order within sections
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @nnethercote! Looks good to me overall.

Let's do a perf run to see if there is a difference due to the case @RalfJung points out.

`use` is a nicer way of doing things.
@nnethercote nnethercote force-pushed the rustc_data_structures-cleanups branch from 76ceba3 to 475c4fe Compare May 8, 2024 04:11
@nnethercote
Copy link
Contributor Author

@michaelwoerister I have addressed the comments. I don't think a perf run is necessary now that I changed the insert(0, ...) calls to push().

@rust-log-analyzer

This comment has been minimized.

@nnethercote nnethercote force-pushed the rustc_data_structures-cleanups branch from 475c4fe to 8b1fec2 Compare May 8, 2024 04:37
@nnethercote nnethercote force-pushed the rustc_data_structures-cleanups branch from 8b1fec2 to 82476c4 Compare May 8, 2024 06:33
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment. With that assertion added, you can r=me.

And move the `repr` line after the `derive` line, where it's harder to
overlook. (I overlooked it initially, and didn't understand how this
type worked.)
It is optimized for lists with a single element, avoiding the need for
an allocation in that case. But `SmallVec<[T; 1]>` also avoids the
allocation, and is better in general: more standard, log2 number of
allocations if the list exceeds one item, and a much more capable API.

This commit removes `TinyList` and converts the two uses to
`SmallVec<[T; 1]>`. It also reorders the `use` items in the relevant
file so they are in just two sections (`pub` and non-`pub`), ordered
alphabetically, instead of many sections. (This is a relevant part of
the change because I had to decide where to add a `use` item for
`SmallVec`.)
It provides a way to effectively embed a linked list within an
`IndexVec` and also iterate over that list. It's written in a very
generic way, involving two traits `Links` and `LinkElem`. But the
`Links` trait is only impl'd for `IndexVec` and `&IndexVec`, and the
whole thing is only used in one module within `rustc_borrowck`. So I
think it's over-engineered and hard to read. Plus it has no comments.

This commit removes it, and adds a (non-generic) local iterator for the
use within `rustc_borrowck`. Much simpler.
It's a macro that just creates an enum with a `from_u32` method. It has
two arms. One is unused and the other has a single use.

This commit inlines that single use and removes the whole macro. This
increases readability because we don't have two different macros
interacting (`enum_from_u32` and `language_item_table`).
@nnethercote nnethercote force-pushed the rustc_data_structures-cleanups branch from 82476c4 to 58a06b6 Compare May 8, 2024 23:08
@nnethercote
Copy link
Contributor Author

@bors r=michaelwoerister

@bors
Copy link
Collaborator

bors commented May 8, 2024

📌 Commit 58a06b6 has been approved by michaelwoerister

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2024
@bors
Copy link
Collaborator

bors commented May 9, 2024

⌛ Testing commit 58a06b6 with merge 37dc766...

@bors
Copy link
Collaborator

bors commented May 9, 2024

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing 37dc766 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 9, 2024
@bors bors merged commit 37dc766 into rust-lang:master May 9, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 9, 2024
@nnethercote nnethercote deleted the rustc_data_structures-cleanups branch May 9, 2024 06:15
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (37dc766): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.2%, 0.5%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.4% [3.5%, 5.4%] 2
Improvements ✅
(primary)
-1.2% [-1.2%, -1.2%] 1
Improvements ✅
(secondary)
-5.9% [-6.2%, -5.5%] 2
All ❌✅ (primary) -1.2% [-1.2%, -1.2%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.709s -> 676.526s (0.12%)
Artifact size: 315.74 MiB -> 315.79 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

Comments